-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a frozendictionary on net-core and a readonly one on netfx #72995
Conversation
Post here when you get results from the test insertions, as I'm curious whether this improved things |
Will do. |
Given stephen's change, this feels slightly less appealing even if the speedometer test results come back with good results |
Note: we'd have to wait for stephen's change to make it all the way out into a release that we can pull into VS. When that happens, i'll undo this an dmove to FrozenDict uniformly. |
Perf tests are here https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/543655 They show that there's no regression at all now that we use a RODict in netfx and a FrozenDict in core: |
Might be nice to slip in a TODO comment in one of your upcoming PRs that this is temporary and a link to Stephen's PR |
I simply will not forget. With my memory being perfect, there's no chance of anything going wrong. |
@jasonmalinowski For review when you get back. |
Running a test insertion to see if this removes the recent regressions caused by: #72965
PR1: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9406987&view=results
PR2: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9407001&view=results
FrozenDicts turn out to be quite expensive on NetFx if you're using StringComparer.CaseInsensitive. It causes thsi sort of blowup as the dict analyzes keys:
This issue is not present on netcore, where FrozenDict can do all of this work in a non-allocating fashion.